fix(scripts): make config integrity check fatal in non-root mode#1032
fix(scripts): make config integrity check fatal in non-root mode#1032Junior00619 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNon-root startup path in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nemoclaw-start.test.js (1)
25-27: Consider a more explicit comment on the regex matching strategy.The regex correctly matches only the outer
fibecause innerfistatements are indented (e.g.,fi) while the block-closingfiat column 0 matches\nfi\n. This is clever but relies on consistent indentation. A brief inline comment explaining this would help future maintainers understand why the regex works.📝 Suggested comment addition
// Extract the non-root block (between "id -u -ne 0" and the matching "fi") + // Note: The \nfi\n pattern matches only the unindented outer "fi" at column 0, + // skipping inner "fi" statements that are indented with leading whitespace. const nonRootMatch = src.match( /if \[ "\$\(id -u\)" -ne 0 \]; then\n([\s\S]*?)\nfi\n/ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nemoclaw-start.test.js` around lines 25 - 27, Add an inline comment above the regex assignment to nonRootMatch explaining the matching strategy: note that the pattern /\nfi\n/ deliberately targets the closing "fi" at column 0 so it matches only the outer block (inner "fi" lines are indented like " fi"), and mention the caveat that this relies on consistent indentation in the source; reference the nonRootMatch variable and the regex /if \[ "\$\(id -u\)" -ne 0 \]; then\n([\s\S]*?)\nfi\n/ so future maintainers understand why the expression safely selects the outer block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/nemoclaw-start.test.js`:
- Around line 25-27: Add an inline comment above the regex assignment to
nonRootMatch explaining the matching strategy: note that the pattern /\nfi\n/
deliberately targets the closing "fi" at column 0 so it matches only the outer
block (inner "fi" lines are indented like " fi"), and mention the caveat that
this relies on consistent indentation in the source; reference the nonRootMatch
variable and the regex /if \[ "\$\(id -u\)" -ne 0 \]; then\n([\s\S]*?)\nfi\n/ so
future maintainers understand why the expression safely selects the outer block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef4136d0-a0ce-487c-8963-a27e4935c57d
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/nemoclaw-start.test.js
In nemoclaw-start.sh, the non-root code path caught verify_config_integrity failures and continued with a warning, bypassing the security model that protects openclaw.json from tampering. The root code path correctly treated the check as fatal (exits under set -euo pipefail). The integrity check is now fatal in both code paths. If the config hash doesn't match, the sandbox refuses to start regardless of whether it's running as root or non-root. Adds regression tests verifying: - The non-root block exits on integrity failure - No code path bypasses verify_config_integrity Fixes NVIDIA#1013
9458309 to
18f12c6
Compare
Summary
Makes the config integrity check fatal in non-root mode. Previously, if
verify_config_integrity()failed in non-root mode, the script logged awarning but continued to start the gateway with potentially tampered config.
Related Issue
Fixes #1013
Changes
root path behavior)
no code path bypasses the integrity check
Type of Change
Code change for a new feature, bug fix, or refactor.
Testing
npm test— 479 passednpx prek run --all-files— all hooks passed (including shellcheck)config integrity check(2 assertions)Summary by CodeRabbit
Bug Fixes
Tests